Skip to content

Fix empty array edge cases in coxphfit - #488

Merged
pr0m1th3as merged 3 commits into
gnu-octave:mainfrom
Sonu0305:fix-coxphfit-empty
Sep 16, 2026
Merged

pr0m1th3as merged 3 commits into
gnu-octave:mainfrom
Sonu0305:fix-coxphfit-empty

Conversation

@Sonu0305

Copy link
Copy Markdown
Contributor

No description provided.

@pr0m1th3as

Copy link
Copy Markdown
Member

MATLAB's result for no data comes from a singular matrix inverse (covb -Inf on the diagonal, se Inf, p and LikelihoodRatioTestP 1), and its "rank deficient" warning names the wrong cause. We don't replicate that, and MATLAB's own mnrfit, mvregress and robustfit error on empty input. Keep coxphfit erroring when no complete observation remains; a clearer message for empty input is welcome. Also, warning ("off", ...) takes an identifier, not the message text, so that test cannot work.

@Sonu0305

Copy link
Copy Markdown
Contributor Author

updated @pr0m1th3as
can you review again, thank you.

@pr0m1th3as

Copy link
Copy Markdown
Member

Please revert the new check on T: for a 3-column T it replaces the clearer existing message about (start, stop] intervals. Dropping the two ! isempty tests is enough.

The empty check runs after the NaN rows are removed, so coxphfit ([NaN; NaN], [1; 2]) now reports empty input instead of "no complete observations remain after removing NaNs". Check straight after the row count check instead, and leave the NaN branch as it was:

  if (rows (X) != n)
    error ("coxphfit: T must have one element for each row of X.");
  endif
  if (n == 0)
    error ("coxphfit: X and T must contain at least one observation.");
  endif

Keep X and T in upper case, give the %!error tests the full message, and write zeros (0, 3) with spaces.

@Sonu0305

Copy link
Copy Markdown
Contributor Author

updated @pr0m1th3as

@pr0m1th3as
pr0m1th3as merged commit 5da713c into gnu-octave:main Sep 16, 2026
@Sonu0305
Sonu0305 deleted the fix-coxphfit-empty branch September 16, 2026 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants